Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/Synthese Export: add group3 inpn into synthese view export #2637

Conversation

andriacap
Copy link
Contributor

Bonjour,

Dans le cadre d'une prestation pour l'Agence Régionale de la Biodiversité en île de France, une demande a été faite pour avoir la possibilité d'afficher la colonne group3_inpn dans les exports de synthese notamment sur les exports csv d'observations et de taxons .

Etant donné que ces téléchargements csv se basent sur les vues v_synthese_for_export et v_synthese_taxon_for_export_view , j'ai suggéré d'ajouter une revision qui modifierait ces vues , ainsi qu'un changement dans la synthese config pour avoir par défaut la colonne group3_inpn dans l'export.

Pour la révision je m'appuie sur des scripts sql plutot que de l'éxécution de "statement" avec alembic . Si la norme veut que l'on fasse autrement , je peux apporter les modifications nécessaires .

Je met la PR en draft en attendant vos retours , avis. D'ailleurs cette PR, bien que indépendante au niveau code, appartient au même "scope" que la PR de Maxime (voir #2621)

Merci

@andriacap andriacap changed the title feat: add group3 inpn into synthese view export Feat/Synthese Export: add group3 inpn into synthese view export Jul 21, 2023
@andriacap andriacap force-pushed the feat/add-group3-inpn-to-synthese-export-view branch from 08ad4f9 to 7263f08 Compare August 1, 2023 13:35
@andriacap andriacap marked this pull request as ready for review August 1, 2023 13:39
@andriacap
Copy link
Contributor Author

Bonjour,

Je ne comprend pas pourquoi les check failed .
J'ai bien récupéré les dev de la branche develop de Geonature et j'ai rebase la branche feat/add-group3-inpn-to-synthese-export-view par rapport à la branche develop.

J'ai également éxécuté les commandes git submodule init , git submodule update et geonature db autoupgrade  . Tout est ok en local , je comprends pas les failed de ces checks ..

En attendant que j'effectue quelques recherches plus approfondies, sii quelqu'un peut m"éclairer afin que les checks passent , ça serait avec plaisir

Merci

@mvergez
Copy link
Contributor

mvergez commented Aug 1, 2023

Il s'avère que la migration ne passe pas car elle dépend de la migration du schéma taxonomie ajoutant la colonne group3_inpn. Il faut donc que celle ci soit une dépendance de la migration créée dans cette PR

@andriacap andriacap force-pushed the feat/add-group3-inpn-to-synthese-export-view branch from e2207ad to bb8393d Compare August 1, 2023 15:24
@andriacap
Copy link
Contributor Author

Merci Maxime pour ton retour !

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a3bde03) 78.44% compared to head (f41151d) 68.56%.
Report is 6 commits behind head on feat/groupinpn3.

❗ Current head f41151d differs from pull request most recent head 64aff5c. Consider uploading reports for the commit 64aff5c to get more accurate results

Additional details and impacted files
@@                 Coverage Diff                 @@
##           feat/groupinpn3    #2637      +/-   ##
===================================================
- Coverage            78.44%   68.56%   -9.88%     
===================================================
  Files                   89       86       -3     
  Lines                 7213     7435     +222     
===================================================
- Hits                  5658     5098     -560     
- Misses                1555     2337     +782     
Flag Coverage Δ
pytest 68.56% <ø> (-9.88%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andriacap andriacap force-pushed the feat/add-group3-inpn-to-synthese-export-view branch from 4a69695 to 39ca1d6 Compare August 11, 2023 07:38
@camillemonchicourt camillemonchicourt added this to the 2.14 milestone Nov 13, 2023
@camillemonchicourt
Copy link
Member

L'idée de créer un fichier SQL pour la création ou modification des vues et d'uniquement exécuter ce SQL dans la migration me plait bien car elle permet plus de lisibilité sur ce qu'on fera ensuite évoluer dans cette vue, avec un diff plus lisible que si on recréé toute la vue dans la migration Alembic.

Cependant on ne fait comme ça jusqu'à présent où l'on recréé justement toute la vue dans la migration quand on fait évoluer une vue ou fonction.

Pourquoi pas partir sur des SQL à part mais à homogénéiser, généraliser et valider ce fonctionnement avant ?
Un fichier par vue ? Un fichier avec toutes les vues ? Un fichier avec toutes les vues et les fonctions ?


Aussi, pour l'ajout du group3_inpn dans la vue, je pense qu'il faut l'ajouter juste après l'ajout des groupes INPN 1 et 2 pour plus de lisibilité.

@andriacap andriacap force-pushed the feat/add-group3-inpn-to-synthese-export-view branch from 39ca1d6 to f485541 Compare November 17, 2023 11:03
@andriacap
Copy link
Contributor Author

L'idée de créer un fichier SQL pour la création ou modification des vues et d'uniquement exécuter ce SQL dans la migration me plait bien car elle permet plus de lisibilité sur ce qu'on fera ensuite évoluer dans cette vue, avec un diff plus lisible que si on recréé toute la vue dans la migration Alembic.

Cependant on ne fait comme ça jusqu'à présent où l'on recréé justement toute la vue dans la migration quand on fait évoluer une vue ou fonction.

Pourquoi pas partir sur des SQL à part mais à homogénéiser, généraliser et valider ce fonctionnement avant ? Un fichier par vue ? Un fichier avec toutes les vues ? Un fichier avec toutes les vues et les fonctions ?

Aussi, pour l'ajout du group3_inpn dans la vue, je pense qu'il faut l'ajouter juste après l'ajout des groupes INPN 1 et 2 pour plus de lisibilité.

Salut @camillemonchicourt , j'ai changé l'ordre d'ajout des colonnes group3_inpn .
Concernant l'architecture d'alembic , il peut être intéressant de ré organiser les fichiers sql dans des sous dossiers également ? (comme j'ai pu le faire dans le dernier commit que je viens de push)

Pour une réflexion plus élargie sur alembic j'ai également vu cette article qui présente des choses qui peuvent être intéressantes à mettre en place --> https://medium.com/alan/making-alembic-migrations-more-team-friendly-e92997f60eb2

@andriacap
Copy link
Contributor Author

Pour le lint frontend , j'ai bien appliqué npm run format et le lint Prettier me dit que tous mes fichiers sont "OK" . Je ne comprend pas pourquoi ça ne passe pas .. J'investigue le problème..

@camillemonchicourt
Copy link
Member

OK, oui intéressant.
Mais, en l'état, ce qui m'embête c'est que le dossier "/backend/geonature/migrations/data/core/" contient les SQL de création des schéma de GN dans l'état en version 2.7, avant de passer à Alembic et n'évoluent plus, mais on commencerait à y ajouter aussi des fichiers SQL qui évoluent, dans un sous-dossier "gn_synthese" avec une partie qui évolue.
Et un fichier SQL de toute la synthèse qui lui n'est pas dans le dossier "gn_synthese" et n'évolue plus (synthese.sql).

Donc si on part sur cette solution, il faudrait trouver un moyen que cela soit plus clair et pas mélanger les anciens trucs qui bougent plus et les SQL des vues et fonctions.

Il y a aussi ce sujet qui reste ouvert sur le fait qu'on a perdu en lisibilité sur la structure de la BDD et de son évolution avec la mise en place d'Alembic (même si on y a beaucoup gagné par ailleurs) - #1574

Dans Geotrek-admin, c'est assez intéressant et clair comme c'est fait :

@andriacap andriacap force-pushed the feat/add-group3-inpn-to-synthese-export-view branch 2 times, most recently from 10c4f11 to f41151d Compare November 20, 2023 15:39
Since the Taxref v14 or v15 , group3_inpn is available

Creation of revision in order to add this column to
v_synthese_for_export and v_synthese_taxon_for_export_view

Reviewed-by: andriacap
Reviewed-by: andriacap
test_synthese export

Reviewed-by: andriacap
Reviewed-by: andriacap
Ajout des colonnes group3_inpn à la suite des autres colonnes
Proposition de ré arrangement des fichiers sql

Reviewed-by: andriacap
Use current way to replace view

Reviewed-by: andriacap
@jacquesfize jacquesfize force-pushed the feat/add-group3-inpn-to-synthese-export-view branch from f41151d to e702400 Compare January 26, 2024 16:03
@jacquesfize jacquesfize changed the base branch from develop to feat/groupinpn3 January 26, 2024 16:29
@jacquesfize jacquesfize mentioned this pull request Feb 9, 2024
4 tasks
@jacquesfize jacquesfize merged commit 0cd44e4 into PnX-SI:feat/groupinpn3 Feb 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants